Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Simplify Commit List sorting logic #8365

Merged
merged 5 commits into from
Apr 19, 2023
Merged

Conversation

bobanm
Copy link
Contributor

@bobanm bobanm commented Apr 15, 2023

What was the problem?

This PR resolves #8364

How was it solved?

  1. Renamed ascendingHeight argument to sortOrder
  2. Changed numerical values of COMMIT_SORT constants, and used it to pass to sort() function callback, instead of using a conditional for checking the sort order.
  3. CommitPool._getAllCommits() no longer takes or passes sorting arguments, as it always uses ascending order, which is the default for CommitList.getAll()
  4. Removed unnecessary conditional in from CommitList.deleteByHeight()

BONUS: Tidied up the comments order and code block spacing in CommitPool class 😎

How was it tested?

All unit tests pass 👌🏻

@bobanm bobanm requested a review from shuse2 April 15, 2023 08:03
@bobanm bobanm self-assigned this Apr 15, 2023
@bobanm bobanm changed the title 8364 commit list sort Simplify Commit List sorting logic Apr 15, 2023
@bobanm bobanm marked this pull request as ready for review April 15, 2023 08:16
@bobanm bobanm requested a review from mosmartin April 15, 2023 08:16
@codecov
Copy link

codecov bot commented Apr 15, 2023

Codecov Report

Merging #8365 (350ebad) into development (ffde487) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 350ebad differs from pull request most recent head c8fc567. Consider uploading reports for the commit c8fc567 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #8365      +/-   ##
===============================================
- Coverage        83.21%   83.21%   -0.01%     
===============================================
  Files              592      592              
  Lines            22192    22188       -4     
  Branches          3249     3244       -5     
===============================================
- Hits             18468    18464       -4     
  Misses            3724     3724              
Impacted Files Coverage Δ
...ne/consensus/certificate_generation/commit_list.ts 100.00% <100.00%> (+2.56%) ⬆️
...ne/consensus/certificate_generation/commit_pool.ts 92.89% <100.00%> (ø)

... and 1 file with indirect coverage changes

Base automatically changed from 8359-fix-single-commit-handling to development April 17, 2023 07:54
@shuse2 shuse2 enabled auto-merge (squash) April 17, 2023 21:35
@shuse2 shuse2 merged commit 998db5f into development Apr 19, 2023
@shuse2 shuse2 deleted the 8364-commit-list-sort branch April 19, 2023 14:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify Commit List sorting logic
3 participants